-
Notifications
You must be signed in to change notification settings - Fork 5
Add CXX child crate #18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Is the plan for dcsctp to be an alternative to https://github.com/sctplab/usrsctp or https://github.com/webrtc-rs/sctp-proto |
c1a35c5 to
4a36400
Compare
dcsctp-cxx/.gitignore
Outdated
| @@ -0,0 +1 @@ | |||
| /examples/main | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move this to the top level .gitignore instead? Makes it easier to maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
dcsctp-cxx/src/lib.rs
Outdated
|
|
||
| fn delete_socket(socket: *mut DcSctpSocket) { | ||
| if !socket.is_null() { | ||
| unsafe { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a // SAFETY: ... comment explaining why this is safe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
dcsctp-cxx/src/lib.rs
Outdated
| if !socket.is_null() { | ||
| unsafe { | ||
| // Re-take ownership of the Box and drop it. | ||
| let _ = Box::from_raw(socket); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you do this instead?
drop(Box::from_raw(socket));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
dcsctp-cxx/src/lib.rs
Outdated
| } | ||
| } | ||
|
|
||
| unsafe fn state(socket: *const DcSctpSocket) -> ffi::SocketState { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you take a &DcSctpSocket instead of *const DcSctpSocket?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done - much better
dcsctp-cxx/src/lib.rs
Outdated
| } | ||
| } | ||
|
|
||
| #[allow(dead_code)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed? It's pub so why would it complain about being unused? Does this have to be pub by the way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's see what CI says now.
This adds CXX FFI for the dcSCTP rust library to integrate it into a C++ application.
Yeah, this is not really the right place, but: This is a Rust version of https://webrtc.googlesource.com/src/+/refs/heads/main/net/dcsctp, and the goal is to make this library have feature and stability parity with that one. Compared to e.g. usrsctp, this library only aims to implement the parts of SCTP that are required to support data channels in WebRTC, And yes, RFC8260 is supported. |
| Box::into_raw(boxed_socket) | ||
| } | ||
|
|
||
| unsafe fn delete_socket(socket: *mut DcSctpSocket) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can move the unsafe block to only be around the drop call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can, but as a raw pointer is passed in, I'm promising a lot. If garbage is passed in, bad things happen, so callers have to be careful and only pass valid data.
A well known LLM strongly suggests I should mark it as unsafe.
dcsctp-cxx/src/lib.rs
Outdated
| use dcsctp::api::SocketState as DcSctpSocketState; | ||
| use std::time::Instant; | ||
|
|
||
| #[cxx::bridge] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you want to add a namespace or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That commit got lost, done.
To avoid polluting the default namespace.
It currently has a single function, to query the library's version, to validate that the C++ FFI is working correctly.